Skip to content

Clear fabric counters queue/port#26

Merged
gechiang merged 1 commit intoAzure:202205from
jfeng-arista:202205-clear-fabric-counters
Sep 26, 2023
Merged

Clear fabric counters queue/port#26
gechiang merged 1 commit intoAzure:202205from
jfeng-arista:202205-clear-fabric-counters

Conversation

@jfeng-arista
Copy link
Contributor

What I did

Raise a PR ( the same as sonic-net/sonic-utilities#2892 ) in the https://github.com/Azure/sonic-utilities.msft/tree/202205

How I did it

How to verify it

tested under https://github.com/Azure/sonic-buildimage-msft/tree/202205

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@jfeng-arista
Copy link
Contributor Author

@gechiang This the clear fabric counters pr for 202205.

@gechiang
Copy link
Contributor

gechiang commented Sep 1, 2023

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

@jfeng-arista
Copy link
Contributor Author

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,

command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892
here it still use the old way
command = "fabricstat -C -q"

I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.

Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

@gechiang
Copy link
Contributor

gechiang commented Sep 1, 2023

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,

command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892 here it still use the old way command = "fabricstat -C -q"

I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.

Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

Thanks. I will have to hold off on picking up this for now until I get the build team to add the necessary PR tests into the pipeline for the MSFT repo so that we can better identify if a PR is really safe without build issues... last time when I picked it up we did not realize there was issue with it until community member and myself started to build with it... I will try to experiment picking up this change in my local branch and attempt a build there to see if the problem is somehow fixed...
Will update you... Thanks!

@jfeng-arista
Copy link
Contributor Author

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,
command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892 here it still use the old way command = "fabricstat -C -q"
I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.
Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

Thanks. I will have to hold off on picking up this for now until I get the build team to add the necessary PR tests into the pipeline for the MSFT repo so that we can better identify if a PR is really safe without build issues... last time when I picked it up we did not realize there was issue with it until community member and myself started to build with it... I will try to experiment picking up this change in my local branch and attempt a build there to see if the problem is somehow fixed... Will update you... Thanks!

thank you

@jfeng-arista
Copy link
Contributor Author

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,
command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892 here it still use the old way command = "fabricstat -C -q"
I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.
Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

Thanks. I will have to hold off on picking up this for now until I get the build team to add the necessary PR tests into the pipeline for the MSFT repo so that we can better identify if a PR is really safe without build issues... last time when I picked it up we did not realize there was issue with it until community member and myself started to build with it... I will try to experiment picking up this change in my local branch and attempt a build there to see if the problem is somehow fixed... Will update you... Thanks!

thank you

I was helped with others here during the weekend to try to pull the full source locally, and the build of broadcom ( which is the one close to upstream ) passed. This makes me more confident about this change within the CURRRENT code base. thank you

@gechiang gechiang merged commit 8cf9817 into Azure:202205 Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants